fix: Don't scan profile events if listener is not set #1686
Conversation
|
Taking a look at the test failures 👀 |
|
I have some tests failing for me on |
erezrokah
force-pushed
the
fix/profile_events_memory
branch
from
b264936 to
ce6ffb5
Compare
|
Anything missing from the PR or from the issue? Is there another approach I should take? 3x improvement in memory allocations seems quite good. I can add more benchmarks/screenshots if needed |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes memory allocation by skipping the scanning and processing of profile events when no profile event listener is registered. The change addresses a performance issue where profile events were being unnecessarily parsed even when the client wasn't interested in them, resulting in significant memory allocations.
Key changes:
- Modified profile event handling to conditionally scan events only when a listener is set
- Refactored
onProcessinitialization to conditionally set theprofileEventshandler
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/issues/1685_test.go | Adds benchmark test to verify the performance improvement for the profile events optimization |
| context.go | Refactors onProcess() to conditionally assign profileEvents handler only when listener is present |
| conn_profile_events.go | Adds scanEvents parameter to skip event parsing when no listener is registered |
| conn_process.go | Updates profile events handling to pass scan flag and conditionally invoke handler |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
| scanEvents := on.profileEvents != nil | ||
| events, err := c.profileEvents(ctx, scanEvents) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| on.profileEvents(events) | ||
| if scanEvents { | ||
| on.profileEvents(events) | ||
| } |
There was a problem hiding this comment.
| scanEvents := on.profileEvents != nil | |
| events, err := c.profileEvents(ctx, scanEvents) | |
| if err != nil { | |
| return err | |
| } | |
| on.profileEvents(events) | |
| if scanEvents { | |
| on.profileEvents(events) | |
| } | |
| if on.profileEvents != nil { | |
| events, err := c.profileEvents(ctx) | |
| if err != nil { | |
| return err | |
| } | |
| on.profileEvents(events) | |
| break | |
| } | |
| c.debugf("[skip profile events. no handler registered]") | |
There was a problem hiding this comment.
This way we don't have to change the c.profileEvents method. We just skip on the consumer side itself. what do you think?
There was a problem hiding this comment.
Thanks for the comment @kavirajk, I tried that initially but it appears we still need to read the block from the server in c.profileEvents(ctx) (we still need to call c.readData(ctx, proto.ServerProfileEvents, false)).
If we could do something like this in queries
....
SETTINGS PROFILE_EVENTS_DISABLED=1
That would be the best I think, but this is probably a bigger change and needs to be done in https://github.com/ClickHouse/ClickHouse
There was a problem hiding this comment.
Regardless of having an option to disable profile events in the session, I think it's good for the client to skip scanning them if the listener is not registered
There was a problem hiding this comment.
I agree! thanks for the clarification @erezrokah 👍 I don't think we have to block this PR for the server settings to be available.
kavirajk
left a comment
kavirajk
left a comment
There was a problem hiding this comment.
Thanks for the PR @erezrokah (also for the benchmarks ❤️ ). Overall looks good. Added couple of small comments. Let me know what do you think.
| scanEvents := on.profileEvents != nil | ||
| events, err := c.profileEvents(ctx, scanEvents) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| on.profileEvents(events) | ||
| if scanEvents { | ||
| on.profileEvents(events) | ||
| } |
There was a problem hiding this comment.
This way we don't have to change the c.profileEvents method. We just skip on the consumer side itself. what do you think?
| } | ||
|
|
||
| func (c *connect) profileEvents(ctx context.Context) ([]ProfileEvent, error) { | ||
| func (c *connect) profileEvents(ctx context.Context, scanEvents bool) ([]ProfileEvent, error) { |
There was a problem hiding this comment.
We may not need to change this profileEvents api at all? . We can skip calling it from the consumer side. See my other comment.
There was a problem hiding this comment.
Replied, I did that initially but it appears we have to consume the data from the server otherwise the querying fails
|
Also related ClickHouse/ClickHouse#80761 |
2 participants
Summary
Fixes #1685. A better way would be to be able to disable these events altogether from being returned to the client.
Also, if this PR is accepted we might want to do the same for other listeners, though I haven't seen a big issue with allocations in other places (see issue for memory profiles).
For the benchmark in the issue this is over a 3x improvement
Checklist